Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APT-576 adding functionality to check if all orgs are the same in foreman.toml #44

Merged
merged 39 commits into from
Oct 3, 2023

Conversation

afujiwara-roblox
Copy link
Contributor

@afujiwara-roblox afujiwara-roblox commented Sep 26, 2023

We want to be able to enforce the same tool source orgs at the setup-foreman step of CI.
This behavior should be the default, but can be bypassed with the allow-external-github-orgs option

@afujiwara-roblox afujiwara-roblox marked this pull request as ready for review September 26, 2023 22:10
Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start so far! There's a crucial detail that I might not have explained well enough that I want to integrate here: we should compare the org in the tools against the org that the current action run is occurring within, not just against each other.

__tests__/hello.test.ts Outdated Show resolved Hide resolved
src/configFile.ts Show resolved Hide resolved
src/configFile.ts Outdated Show resolved Hide resolved
src/configFile.ts Outdated Show resolved Hide resolved
src/configFile.ts Outdated Show resolved Hide resolved
src/configFile.ts Outdated Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
.github/workflows/test.yml Show resolved Hide resolved
src/main.ts Outdated Show resolved Hide resolved
src/configFile.ts Outdated Show resolved Hide resolved
src/configFile.ts Show resolved Hide resolved
src/configFile.ts Outdated Show resolved Hide resolved
src/configFile.ts Outdated Show resolved Hide resolved
src/configFile.ts Show resolved Hide resolved
}
const manifestContent = parse(data);
const sameGithubOrgSource = checkSameOrgToolSpecs(manifestContent, org);
if (sameGithubOrgSource == false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (sameGithubOrgSource == false) {
if (!sameGithubOrgSource) {

general practice for all languages (especially C/C++) is to never compare bools on equality to bool constants
in JS null, undefined and false are interchangeable in different functions.
It is strongly recommended to use if (var) and if (!var) to make code change proof

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we need to add linter...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This repo uses eslint

src/configFile.ts Outdated Show resolved Hide resolved
@afujiwara-roblox
Copy link
Contributor Author

Anton and I synced offline and it seems like explicit null checks are recommended https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/strict-boolean-expressions.md

Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thanks for making the improvements!

@afujiwara-roblox afujiwara-roblox merged commit a27b8a4 into master Oct 3, 2023
9 checks passed
@afujiwara-roblox afujiwara-roblox deleted the APT-576 branch October 3, 2023 23:34
@github-actions github-actions bot locked and limited conversation to collaborators Oct 3, 2023
@afujiwara-roblox afujiwara-roblox restored the APT-576 branch October 9, 2023 22:40
@cliffchapmanrbx cliffchapmanrbx deleted the APT-576 branch December 16, 2023 01:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants